Skip to content

feat(redis): Set db.query.text span attributes#6639

Merged
ericapisani merged 3 commits into
masterfrom
py-2549-set-db-attr-redis
Jun 25, 2026
Merged

feat(redis): Set db.query.text span attributes#6639
ericapisani merged 3 commits into
masterfrom
py-2549-set-db-attr-redis

Conversation

@ericapisani

@ericapisani ericapisani commented Jun 23, 2026

Copy link
Copy Markdown
Member

When using the span streaming path, db.query.text was not being set on db.redis spans

This adds SPANDATA.DB_QUERY_TEXT to spans in both the sync and async Redis common modules, and extends existing tests to assert these attributes are present.

Fixes PY-2549
Fixes #6638

When using span streaming, set SPANDATA.DB_QUERY_TEXT on db.redis spans
and SPANDATA.CACHE_KEY on cache spans. These were already set as the span
description/name but were missing from the attributes dict in the streaming
path.

Refs PY-2549
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jun 23, 2026

Copy link
Copy Markdown

PY-2549

@ericapisani ericapisani marked this pull request as ready for review June 23, 2026 18:59
@ericapisani ericapisani requested a review from a team as a code owner June 23, 2026 18:59
@@ -116,6 +116,7 @@ def sentry_patched_execute_command(
attributes={
"sentry.op": cache_properties["op"],
"sentry.origin": SPAN_ORIGIN,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The cache span is initialized with SPANDATA.CACHE_KEY set to the description string, not the key tuple, causing an incorrect attribute type in exception paths.
Severity: LOW

Suggested Fix

Modify the span creation to use the correct property from the start. Instead of setting SPANDATA.CACHE_KEY to cache_properties["description"], set it to cache_properties["key"]. This ensures the attribute has the correct type (a tuple of strings) from the moment of initialization, even in exception paths.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry_sdk/integrations/redis/_sync_common.py#L118

Potential issue: The code in `_sync_common.py` and `_async_common.py` incorrectly
initializes the `SPANDATA.CACHE_KEY` attribute on the cache span with
`cache_properties["description"]`, which is a string. The correct value should be
`cache_properties["key"]`, a tuple of strings. While a subsequent call to
`_set_cache_data` overwrites this with the correct value in the normal execution flow,
this correction does not happen if an exception is raised during the Redis command
execution. In such an exception scenario, the span would be captured with the wrong
attribute type (a string instead of a list of strings), leading to inconsistent
telemetry data.

Also affects:

  • sentry_sdk/integrations/redis/_async_common.py:119~119

Did we get this right? 👍 / 👎 to inform future reviews.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Codecov Results 📊

89837 passed | ⏭️ 6240 skipped | Total: 96077 | Pass Rate: 93.51% | Execution Time: 319m 40s

📊 Comparison with Base Branch

Metric Change
Total Tests 📉 -63
Passed Tests 📉 -63
Failed Tests
Skipped Tests

All tests are passing successfully.

✅ Patch coverage is 100.00%. Project has 2404 uncovered lines.
❌ Project coverage is 89.89%. Comparing base (base) to head (head).

Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
- Coverage    89.90%    89.89%    -0.01%
==========================================
  Files          192       192         —
  Lines        23772     23775        +3
  Branches      8212      8206        -6
==========================================
+ Hits         21370     21371        +1
- Misses        2402      2404        +2
- Partials      1341      1344        +3

Generated by Codecov Action

Comment thread sentry_sdk/integrations/redis/_async_common.py Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c758f7f. Configure here.

with capture_internal_exceptions():
additional_cache_span_attributes[SPANDATA.DB_QUERY_TEXT] = (
_get_safe_command(name, args)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache spans use db.query.text

Medium Severity

For span streaming, cache spans merge additional_cache_span_attributes using SPANDATA.DB_QUERY_TEXT and _get_safe_command, but this change was meant to populate cache.key on cache spans. That mislabels cache.get / cache.put spans with a database query attribute and does not add cache.key at span creation (only later via _set_cache_data).

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c758f7f. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment still needs to be addressed @ericapisani

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was to address your earlier comment re: setting the full redis command without truncation as an attribute.

As part of this, I reversed the earlier change I had made introducing the cache.key attribute, so this bot comment is coming up because I forgot to update the title/description of the PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool another case of the bots being confused 😞. Approved now!

Comment thread tests/integrations/redis/test_redis_cache_module.py
@ericapisani ericapisani changed the title feat(redis): Set db.query.text and cache.key span attributes feat(redis): Set db.query.text span attributes Jun 25, 2026
@ericapisani ericapisani merged commit ad67968 into master Jun 25, 2026
265 of 267 checks passed
@ericapisani ericapisani deleted the py-2549-set-db-attr-redis branch June 25, 2026 13:10
Comment on lines +339 to 344
assert (
set1["attributes"][SPANDATA.DB_QUERY_TEXT]
== f"SET 'somekey1' '{long_string}'"
)
assert set1["attributes"]["sentry.op"] == "db.redis"
assert set2["name"] == expected_short

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB_QUERY_TEXT span attribute bypasses user-configured max_data_size limit

In the span-streaming path, db.redis spans now set SPANDATA.DB_QUERY_TEXT to the raw output of _get_safe_command(name, args). Unlike the span description/name (computed by _get_db_span_description), this value is never truncated to integration.max_data_size. With send_default_pii=True, large Redis values (e.g. a 100,000-char SET payload) are stored verbatim in the attribute, producing very large span attributes that bypass the user's configured data-size cap and risk exceeding Sentry's ingest payload limits. The same pattern applies to the cache-key attribute in both _sync_common.py and _async_common.py.

Evidence
  • _get_db_span_description in modules/queries.py truncates the span name to integration.max_data_size (description[: max_data_size - 3] + '...'), and _get_cache_span_description in modules/caches.py does the same.
  • _get_safe_command in redis/utils.py only limits arg count (_MAX_NUM_ARGS) and never truncates individual arg length or applies max_data_size.
  • _sync_common.py:141 (and the equivalent in _async_common.py) assigns additional_db_span_attributes[SPANDATA.DB_QUERY_TEXT] = _get_safe_command(name, args) and passes it straight into the streamed span attributes.
  • The new test test_data_truncation_custom (test_redis.py:339-342) configures max_data_size=30 yet asserts set1["attributes"][SPANDATA.DB_QUERY_TEXT] == f"SET 'somekey1' '{long_string}'" where long_string = 'a' * 100000, confirming the ~100k-char value is stored untruncated while the span name is truncated to 30.

Identified by Warden code-review · JGL-PYX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set db.query.text attribute on redis streamed spans

2 participants